Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

PHP Gherkin Parser: v24 is not compatible with v18 of messages #2034

Merged
merged 3 commits into from
Jul 4, 2022
Merged

PHP Gherkin Parser: v24 is not compatible with v18 of messages #2034

merged 3 commits into from
Jul 4, 2022

Conversation

dantleech
Copy link
Contributor

Summary

Updated composer.json to require at least v19 of cubumber PHP messages

Details

When using v24 with lowest dependencies:

cucumber/gherkin                      v24.0.0     Gherkin parser
cucumber/messages                     v18.0.0     JSON schema-based messages for Cucumber's inter-process communication

We get the following error:

PHP Fatal error:  Uncaught Error: Class "Cucumber\Messages\Step\KeywordType" not found in /home/daniel/www/dantleech/gherkin-lint/vendor/cucumber/gherkin/src/GherkinParser.php:38

With v19.0.0 of cucumber/messages it works.

Types of changes

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).

@dantleech dantleech changed the title v24 is not compatible with v18 of messages PHP Gherkin Parser: v24 is not compatible with v18 of messages Jul 3, 2022
@dantleech
Copy link
Contributor Author

cc @ciaranmcnulty

@ciaranmcnulty
Copy link
Contributor

ciaranmcnulty commented Jul 3, 2022 •
edited
Loading

Thanks @dantleech that's annoying

What would be the best away to detect this? Add composer install --prefer-lowest to the CI setup?

@dantleech
Copy link
Contributor Author

yes, that's what I'm doing with GH actions

@aurelien-reeves
Copy link
Contributor

Thanks @dantleech for the patch 👍
@ciaranmcnulty is it fine? Can we merge?

aurelien-reeves
aurelien-reeves previously requested changes Jul 4, 2022
View reviewed changes
Copy link
Contributor

@aurelien-reeves aurelien-reeves left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An entry in the changelog is missing actually ☺️

@aurelien-reeves
Copy link
Contributor

Do we try to add a non-regression test as part of this PR? Or do we quickly release the patch asap and work on a new test in another PR?

@ciaranmcnulty
Copy link
Contributor

I think it should be merged with changelog entry

I am so confused by the Makefiles in this repo that I'm to sure how to add a second job effectively :/ It basically needs a second CI job that can run composer update --prefer-lowest either initially, or later on (to downgrade dependencies) before running tests

@aurelien-reeves
Copy link
Contributor

I think it should be merged with changelog entry

I am so confused by the Makefiles in this repo that I'm to sure how to add a second job effectively :/ It basically needs a second CI job that can run composer update --prefer-lowest either initially, or later on (to downgrade dependencies) before running tests

I'll take a look as soon as I have a moment 👌

@ciaranmcnulty
Copy link
Contributor

@aurelien-reeves we could defer until repo split puts it all in GHA ;)

ciaranmcnulty added a commit that referenced this pull request Jul 4, 2022
@ciaranmcnulty
Check lowest versions of dependencies
fb496cd 
This is mostly because of #2034 - we need to check that
everything still works with the 'lowest' versions of the dependencies

There aren't any external dependencies yet so currently this will only detect when Gherkin doesn't work with Messages any more, or when we've written tests that don't work with older versions of the test tools
@ciaranmcnulty ciaranmcnulty mentioned this pull request Jul 4, 2022
Merged
ciaranmcnulty added a commit that referenced this pull request Jul 4, 2022
@ciaranmcnulty
Check lowest versions of dependencies
efb2e61 
This is mostly because of #2034 - we need to check that
everything still works with the 'lowest' versions of the dependencies

There aren't any external dependencies yet so currently this will only detect when Gherkin doesn't work with Messages any more, or when we've written tests that don't work with older versions of the test tools
@ciaranmcnulty
Copy link
Contributor

@aurelien-reeves actually by explaining it I realised maybe it's easy #2035

ciaranmcnulty added a commit that referenced this pull request Jul 4, 2022
@ciaranmcnulty
Check lowest versions of dependencies
8433241 
This is mostly because of #2034 - we need to check that
everything still works with the 'lowest' versions of the dependencies

There aren't any external dependencies yet so currently this will only detect when Gherkin doesn't work with Messages any more, or when we've written tests that don't work with older versions of the test tools
ciaranmcnulty added a commit that referenced this pull request Jul 4, 2022
@ciaranmcnulty
Check lowest versions of dependencies
4652afe 
This is mostly because of #2034 - we need to check that
everything still works with the 'lowest' versions of the dependencies

There aren't any external dependencies yet so currently this will only detect when Gherkin doesn't work with Messages any more, or when we've written tests that don't work with older versions of the test tools
ciaranmcnulty added a commit that referenced this pull request Jul 4, 2022
@ciaranmcnulty
Check lowest versions of dependencies
44ea4d5 
This is mostly because of #2034 - we need to check that
everything still works with the 'lowest' versions of the dependencies

There aren't any external dependencies yet so currently this will only detect when Gherkin doesn't work with Messages any more, or when we've written tests that don't work with older versions of the test tools
@ciaranmcnulty
Copy link
Contributor

Before merging this I'd like to have a small stab at making it backwardly compatible

@ciaranmcnulty
Copy link
Contributor

Oh wait the testdata has the new fields in, no chance :)

@ciaranmcnulty ciaranmcnulty enabled auto-merge (squash) July 4, 2022 11:19
@ciaranmcnulty ciaranmcnulty enabled auto-merge (squash) July 4, 2022 11:24
@aurelien-reeves
Copy link
Contributor

Before merging this I'd like to have a small stab at making it backwardly compatible

I would have been pleased to find some ways to make it backward compatible indeed.
But as long as this has already been released, I think it is too late.

Having distinct repos in a near future will certainly help improving backward compatibility when working on new features like we did

@ciaranmcnulty ciaranmcnulty merged commit ec4bcc4 into cucumber:main Jul 4, 2022
@aslakhellesoy
Copy link
Contributor

Hi @dantleech,

Thanks for your making your first contribution to Cucumber, and welcome to the Cucumber committers team! You can now push directly to this repo and all other repos under the cucumber organization! 🍾

In return for this generous offer we hope you will:

  • ✅ Continue to use branches and pull requests. When someone on the core team approves a pull request (yours or someone else's), you're welcome to merge it yourself.
  • 💚 Commit to setting a good example by following and upholding our code of conduct in your interactions with other collaborators and users.
  • 💬 Join the community Slack channel to meet the rest of the team and make yourself at home.
  • ℹ️ Don't feel obliged to help, just do what you can if you have the time and the energy.
  • 🙋 Ask if you need anything. We're looking for feedback about how to make the project more welcoming, so please tell us!

On behalf of the Cucumber core team,
Aslak Hellesøy
Creator of Cucumber

ciaranmcnulty added a commit that referenced this pull request Jul 4, 2022
@ciaranmcnulty
Check lowest versions of dependencies
1323e6a 
This is mostly because of #2034 - we need to check that
everything still works with the 'lowest' versions of the dependencies

There aren't any external dependencies yet so currently this will only detect when Gherkin doesn't work with Messages any more, or when we've written tests that don't work with older versions of the test tools
aurelien-reeves added a commit that referenced this pull request Jul 4, 2022
@ciaranmcnulty @aurelien-reeves
PHP: Check lowest versions of dependencies in CI (#2035)
bacd05a 
* Check lowest versions of dependencies

This is mostly because of #2034 - we need to check that
everything still works with the 'lowest' versions of the dependencies

There aren't any external dependencies yet so currently this will only detect when Gherkin doesn't work with Messages any more, or when we've written tests that don't work with older versions of the test tools

* Fix COMPOSER_FLAGS var in Makefile

* Switch install to update as we gitignore composer.lock

* Attempt to fix php 'bc' builds

* Update ramsey/uuid to minimum 4.2.1

The following fix is needed: ramsey/uuid#383

* Explicitly depend on high version of nikic/php-parser

This is used in static analysis and older versions can't deal with enums directly

Becuase we don't depend on it directly Psalm was allowing an older version to install

* Silence a static analysis error in tests

This is something we still want to check for even if the types say it's redundant - JSON decoding could be
throwing an error here

* Remove path repository for low-dependency build

Co-authored-by: aurelien-reeves <[email protected]>
cukebot pushed a commit to cucumber/gherkin-php that referenced this pull request Jul 4, 2022
@ciaranmcnulty @aurelien-reeves
PHP: Check lowest versions of dependencies in CI (#2035)
3905f03 
* Check lowest versions of dependencies

This is mostly because of cucumber/common#2034 - we need to check that
everything still works with the 'lowest' versions of the dependencies

There aren't any external dependencies yet so currently this will only detect when Gherkin doesn't work with Messages any more, or when we've written tests that don't work with older versions of the test tools

* Fix COMPOSER_FLAGS var in Makefile

* Switch install to update as we gitignore composer.lock

* Attempt to fix php 'bc' builds

* Update ramsey/uuid to minimum 4.2.1

The following fix is needed: ramsey/uuid#383

* Explicitly depend on high version of nikic/php-parser

This is used in static analysis and older versions can't deal with enums directly

Becuase we don't depend on it directly Psalm was allowing an older version to install

* Silence a static analysis error in tests

This is something we still want to check for even if the types say it's redundant - JSON decoding could be
throwing an error here

* Remove path repository for low-dependency build

Co-authored-by: aurelien-reeves <[email protected]>
cukebot pushed a commit to cucumber/messages-php that referenced this pull request Jul 4, 2022
@ciaranmcnulty @aurelien-reeves
PHP: Check lowest versions of dependencies in CI (#2035)
6dffdb8 
* Check lowest versions of dependencies

This is mostly because of cucumber/common#2034 - we need to check that
everything still works with the 'lowest' versions of the dependencies

There aren't any external dependencies yet so currently this will only detect when Gherkin doesn't work with Messages any more, or when we've written tests that don't work with older versions of the test tools

* Fix COMPOSER_FLAGS var in Makefile

* Switch install to update as we gitignore composer.lock

* Attempt to fix php 'bc' builds

* Update ramsey/uuid to minimum 4.2.1

The following fix is needed: ramsey/uuid#383

* Explicitly depend on high version of nikic/php-parser

This is used in static analysis and older versions can't deal with enums directly

Becuase we don't depend on it directly Psalm was allowing an older version to install

* Silence a static analysis error in tests

This is something we still want to check for even if the types say it's redundant - JSON decoding could be
throwing an error here

* Remove path repository for low-dependency build

Co-authored-by: aurelien-reeves <[email protected]>
@dantleech dantleech deleted the fixdep branch July 4, 2022 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aurelien-reeves aurelien-reeves aurelien-reeves left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dantleech @ciaranmcnulty @aurelien-reeves @aslakhellesoy